Skip to content

Conversation

@deansheather
Copy link
Member

Users want to create workspaces with branch names like feature/foo, bugfix/bar, docs/api/endpoints - common in Git workflows. Previously blocked because slashes aren't filesystem-safe.

Solution

Allow slashes in branch names but sanitize them for directory names (/-). Git commands use the actual branch name; filesystem operations use the sanitized version.

Key insight: Config stores only the branch name. Directory names are ALWAYS computed on-the-fly via sanitizeBranchNameForDirectory(). Never stored separately.

Changes (~120 LoC)

  • New utility (src/utils/workspace/directoryName.ts): Sanitization + conflict detection
  • Validation update: Allow forward slashes, still reject backslashes
  • IPC layer: Conflict checks before create/rename
  • Runtime: Use directoryName param (sanitized) for paths, branchName for git
  • Special handling: No-op renames when sanitized names match (e.g., docs-foodocs/foo)

Conflict Detection

Prevents collisions like:

  • feature/foo + existing feature-foo → ERROR (both use directory feature-foo)
  • docs/api/guide + existing docs-api-guide → ERROR

Tests

  • Unit: 31 tests for sanitization + validation
  • Integration: 6 tests for creation, conflicts, rename with slashes
  • All passing ✅

Generated with cmux

- Add sanitization utility that converts slashes to dashes for directory names
- Update validation to allow forward slashes (but still reject backslashes)
- Add conflict detection for cases like feature/foo vs feature-foo
- Update runtime implementations to use sanitized directory names
- Handle no-op renames when sanitized names are identical
- Add comprehensive unit and integration tests

Users can now create workspaces with names like feature/new-ui, bugfix/issue-123,
docs/api/endpoints, etc. Git operations use the actual branch name (with slashes)
while filesystem operations use the sanitized version (slashes → dashes).

_Generated with `cmux`_
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

https://github.com/coder/cmux/blob/907e291120db7ba1e8708a567e12eebb04e42929/src/runtime/LocalRuntime.ts#L481-L496
P1 Badge Sanitize branch names when deleting local workspaces

Workspaces created through IPC_CHANNELS.WORKSPACE_CREATE now store directories under sanitizeBranchNameForDirectory(branchName), but LocalRuntime.deleteWorkspace still passes the raw workspaceName directly into getWorkspacePath. When the branch name contains /, the computed path points to a non‑existent nested directory (e.g. feature/foo instead of feature-foo) and the deletion command fails, leaving the worktree on disk. Any attempt to delete or clean up a workspace whose name includes slashes will error out or leak the directory.


https://github.com/coder/cmux/blob/907e291120db7ba1e8708a567e12eebb04e42929/src/runtime/SSHRuntime.ts#L925-L928
P1 Badge SSH workspace rename uses unsanitized paths

The SSH runtime now creates workspaces under sanitized directory names, but SSHRuntime.renameWorkspace still derives oldPath and newPath from the raw branch names. For names containing /, the computed paths will not match the actual directories (which replaced slashes with dashes), so the mv command fails and the workspace cannot be renamed. Remote runtimes therefore cannot rename slash‑named workspaces even though creation is allowed.


https://github.com/coder/cmux/blob/907e291120db7ba1e8708a567e12eebb04e42929/src/runtime/SSHRuntime.ts#L986-L988
P1 Badge SSH workspace deletion ignores directory sanitization

Similarly, SSHRuntime.deleteWorkspace still calls getWorkspacePath(projectPath, workspaceName) with the raw branch name. After permitting / in names, the runtime created directories with slashes converted to dashes, so deleting a slash‑named workspace now targets a different path and the pre‑deletion checks and removal commands fail. Remote workspace deletion for these names will consistently error or leave directories undeleted.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Address Codex review feedback:
- LocalRuntime.deleteWorkspace now sanitizes workspaceName before computing path
- SSHRuntime.renameWorkspace now sanitizes both old and new names
- SSHRuntime.deleteWorkspace now sanitizes workspaceName before computing path

Without sanitization, operations on workspaces with slashes would fail because
the computed paths wouldn't match the actual directory names (which have
slashes converted to dashes).

_Generated with `cmux`_
@deansheather
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/services/ipcMain.ts#L942-L950
P0 Badge Sanitize workspace name before computing bash cwd

The new create path uses sanitizeBranchNameForDirectory, but WORKSPACE_EXECUTE_BASH still calls runtime.getWorkspacePath(metadata.projectPath, metadata.name). When metadata.name contains a slash (now allowed), this reconstructs a path with embedded directories (project/feature/foo) while the worktree was created at the sanitized path (project/feature-foo). Every bash execution for such workspaces will run in a non‑existent directory and fail. Use the sanitized directory name (or sanitize inside getWorkspacePath) before building workspacePath.


https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/services/aiService.ts#L637-L646
P0 Badge AI session resolves incorrect path for slash branch names

The system message setup still derives the workspace directory with runtime.getWorkspacePath(metadata.projectPath, metadata.name). After this commit worktrees are created using the sanitized directory name (slashes → dashes). For branch names like feature/foo, the reconstructed path will be …/feature/foo, which does not exist, causing tool initialization and system message generation to fail. The branch name should be sanitized before calling getWorkspacePath or the runtime helper should handle sanitization internally.


https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/services/agentSession.ts#L184-L194
P0 Badge Agent session path assertion fails for slash names

During workspace registration the code recomputes the expected path via runtime.getWorkspacePath(metadata.projectPath, metadata.name) and asserts it matches the supplied path. With the new sanitization, workspaces created for feature/foo live at …/feature-foo; the recomputed path still uses the unsanitized branch, so the assertion will fire on subsequent registrations (e.g., app restart) and the session will not initialize. Sanitize metadata.name before calling getWorkspacePath or make getWorkspacePath perform the conversion.


https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/runtime/LocalRuntime.ts#L584-L588
P1 Badge Forking uses unsanitized source path

Local workspace forking still calls this.getWorkspacePath(projectPath, sourceWorkspaceName) where sourceWorkspaceName is the branch name from config. When that name contains slashes, this constructs project/feature/foo even though the workspace lives at project/feature-foo; the subsequent git branch --show-current executes in a non‑existent directory and the fork fails. Sanitize sourceWorkspaceName (and probably make getWorkspacePath handle sanitization) before resolving the path.


https://github.com/coder/cmux/blob/7f5eb7eda2a4b6f45922c933de121cb8b16bf4c2/src/config.ts#L370-L375
P1 Badge Forked workspace paths stored unsanitized in config

addWorkspace still derives workspacePath as path.join(this.srcDir, projectName, metadata.name). When IPC_WORKSPACE_FORK passes a branch like docs/api, the saved path becomes …/docs/api while the runtime created …/docs-api. The config entry will not match the actual directory, breaking later lookups that rely on the stored path. Compute the path with the sanitized directory name or use the path returned by the runtime instead of reconstructing it here.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Address Codex P0/P1 issues:
- IPC WORKSPACE_EXECUTE_BASH: sanitize before computing bash cwd
- AI session setup: sanitize before resolving workspace path
- Agent session registration: sanitize before path assertion
- Local fork: sanitize source workspace name
- Config addWorkspace: sanitize when computing workspace path

Without these fixes, any operation involving workspaces with slashes in their
names would use the wrong paths (e.g., 'feature/foo' instead of 'feature-foo')
causing bash execution, AI tool initialization, session registration, and
forking to fail.

_Generated with `cmux`_
@deansheather
Copy link
Member Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 643 to 650
const isInPlace = metadata.projectPath === metadata.name;
const workspacePath = isInPlace
? metadata.projectPath
: runtime.getWorkspacePath(metadata.projectPath, metadata.name);
: (() => {
// Sanitize branch name for directory path
const { sanitizeBranchNameForDirectory } = require("@/utils/workspace/directoryName");
const directoryName = sanitizeBranchNameForDirectory(metadata.name);
return runtime.getWorkspacePath(metadata.projectPath, directoryName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use relative path for runtime sanitization require

The new workspace-path logic calls require("@/utils/workspace/directoryName") inside AIService. Unlike static import statements, this dynamic require executes at runtime and is not processed by the TypeScript path alias resolver, so Node will look for a package literally named @ and throw MODULE_NOT_FOUND. That means every call to build the system message for a workspace (almost all non in-place workspaces) will crash before it can sanitize the branch name. Use a relative path (e.g. require("../utils/workspace/directoryName")) or convert the call to a top-level import to ensure it resolves correctly.

Useful? React with 👍 / 👎.

Address recurring bug pattern where callers must remember to sanitize branch
names before calling getWorkspacePath. This was error-prone and already missed
in ~8 places.

Solution: Make getWorkspacePath() always sanitize its input. This is:
- Idempotent: sanitizing an already-sanitized name returns the same value
- Safe: ensures all 23 call sites automatically get correct paths
- Future-proof: single point of truth for sanitization logic

Now developers can call runtime.getWorkspacePath(projectPath, branchName)
without worrying about sanitization - it happens automatically.

_Generated with `cmux`_
@deansheather
Copy link
Member Author

💡 Better Solution: Sanitization in getWorkspacePath

Great catch on the bug-prone pattern! Instead of requiring every caller to remember to sanitize, I've moved the sanitization inside getWorkspacePath itself.

Before (error-prone):

// Had to remember to sanitize at 23+ call sites
const directoryName = sanitizeBranchNameForDirectory(metadata.name);
const path = runtime.getWorkspacePath(projectPath, directoryName);

After (automatic):

// Just works - sanitization happens automatically
const path = runtime.getWorkspacePath(projectPath, metadata.name);

Why This Works:

  • Idempotent: Sanitizing an already-sanitized name returns the same value
  • Safe: All 23 call sites now automatically get correct paths
  • Single source of truth: Only one place to change sanitization logic
  • Future-proof: No chance of missing sanitization in new code

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant